-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ref(gsAdmin): Rewrite <ForkCustomerAction>
as a functional component
#95399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref(gsAdmin): Rewrite <ForkCustomerAction>
as a functional component
#95399
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95399 +/- ##
==========================================
- Coverage 87.84% 87.79% -0.05%
==========================================
Files 10502 10495 -7
Lines 608126 606018 -2108
Branches 23719 23719
==========================================
- Hits 534228 532081 -2147
- Misses 73540 73579 +39
Partials 358 358 |
// TODO: We should make sure that `setConfirmCallback` is a stable function | ||
// before passing it in here. But because it's not memoized right now, we | ||
// need to store it in a ref between renders. | ||
const onConfirmRef = useRef(setConfirmCallback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stale Callbacks in Mutation Hooks
The onConfirmRef
captures a stale setConfirmCallback
prop, preventing the component from registering updated confirmation callbacks from the parent. This also leads to setConfirmCallback
being called repeatedly whenever the mutate
function (a useEffect
dependency) is recreated. Additionally, the useMutation
callbacks (mutationFn
, onSuccess
, onError
) capture stale values (e.g., regionUrl
, onConfirm
) from their closure. Finally, the mutationFn
is defined as accepting no parameters, despite the useMutation
type signature indicating it should accept AdminConfirmParams
.
static/gsAdmin/components/forkCustomer.tsx#L34-L66
sentry/static/gsAdmin/components/forkCustomer.tsx
Lines 34 to 66 in 63a1c4f
// need to store it in a ref between renders. | |
const onConfirmRef = useRef(setConfirmCallback); | |
const [regionUrl, setRegionUrl] = useState(''); | |
const api = useApi({persistInFlight: true}); | |
const navigate = useNavigate(); | |
const {mutate} = useMutation<any, RequestError, AdminConfirmParams>({ | |
mutationFn: () => { | |
const regions = getRegions(); | |
const region = regions.find(r => r.url === regionUrl); | |
return api.requestPromise(`/organizations/${organization.slug}/fork/`, { | |
method: 'POST', | |
host: region?.url, | |
}); | |
}, | |
onSuccess: (response, params) => { | |
const regions = getRegions(); | |
const region = regions.find(r => r.url === regionUrl); | |
navigate(`/_admin/relocations/${region?.name}/${response.uuid}/`); | |
onConfirm?.({regionUrl, ...params}); | |
}, | |
onError: (error: RequestError, _params) => { | |
if (error.responseJSON) { | |
onConfirm?.({error}); | |
} | |
}, | |
}); | |
useEffect(() => { | |
onConfirmRef.current(mutate); | |
}, [mutate]); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Cursor helped a lot here
Related to https://github.com/getsentry/frontend-tsc/issues/42